-
Notifications
You must be signed in to change notification settings - Fork 5
Copier Enhancement: Multi-Org Support and Workflow-Based Configs #94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
dacharyc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a handful of comments and suggestions amounting to two things:
- If we want to go with this workflow-based approach instead of the "legacy" rules-based approach, I think we should make it a clean refactor and require the workflow-based approach, removing all support for the rules-based approach. AFAIK we're only using that in one repo, which should be easy enough to update, and it simplifies maintenance if we can just have the one path going forward.
- I have reservations about requiring configs to live in a centralized config repo. Requiring separate updates to some other repo is a good way to get drift between the source code and the config that copies it. i.e. when we add other apps, there is no config file in the sample app repo reminding us to update the config. That requires a separate PR somewhere else decoupled from the source code. I feel strongly about it, but I know you prefer this centralized config repo approach, so I don't want to block your PR just because I arbitrarily feel a way about this. I just wanted to raise the concerns here, so if we do want to proceed with this approach, we can acknowledge this is an intentional tradeoff we're making for sound reasons.
examples-copier/configs/copier-config-examples/copier-config-workflow.yaml
Outdated
Show resolved
Hide resolved
examples-copier/configs/copier-config-examples/copier-config-workflow.yaml
Outdated
Show resolved
Hide resolved
examples-copier/configs/copier-config-examples/multi-org-config-example.yaml
Show resolved
Hide resolved
1047eb7 to
4395613
Compare
|
@dacharyc I've implemented the decoupled workflow-style config, and removed as much of the legacy code (or fallback, backward compatible, conversion, etc.) that I could find. I've tested it out on my |
dacharyc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reservations about the complexity we're introducing here with the copier behavior potentially being spread across many files making it more difficult to understand and debug, but that's not blocking feedback, so ✅ with a couple of comments/Qs if you think this implementation is the best way to meet our needs.
.copier/workflows/main.yaml
Outdated
| repo: "mongodb/docs-sample-apps" | ||
| branch: "main" # optional, defaults to main | ||
| path: ".copier/workflows.yaml" | ||
| path: ".copier/workflows/config.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I find this naming a little confusing. We've got a workflows directory (plural) but it contains a singular config.yaml - and the naming is generic enough that there's no easy way to distinguish it from other workflows if we were to add multiple configs in the future. And it looks like we're hard-coding a singular path for the source/repo/branch. So it seems this implementation will only support a singular workflow file per branch.
So I'm wondering if this should be named something like .copier/workflows/main.yaml since it's the workflow for the main branch, or if we expect each repo to realistically have only one workflow, omit the workflows directory entirely and just make it something like .copier/config.yaml?
examples-copier/configs/copier-config-examples/MAIN-CONFIG-README.md
Outdated
Show resolved
Hide resolved
- Renamed REPO_OWNER → CONFIG_REPO_OWNER - Renamed REPO_NAME → CONFIG_REPO_NAME - Renamed SRC_BRANCH → CONFIG_REPO_BRANCH This makes it clear these variables are for the config repository (where copier-config.yaml is stored), not for source code repos. Updated: - Config struct and constants in environment.go - All service files that reference these variables - All test files - All config examples (.env.local.example, env.yaml.example, env.yaml.production) Breaking change: Deployments must update their env.yaml files with new variable names.
- Add pagination support to GetFilesChangedInPr() to handle PRs with >100 files - Update PullRequestQuery struct with PageInfo (EndCursor, HasNextPage) - Implement pagination loop to fetch all files across multiple pages - Update function signature to accept owner/repo parameters - Fix PR body template rendering in workflow processor - Add MessageTemplater to workflow processor dependencies - Render commit message, PR title, and PR body with MessageContext - Replace template variables with actual values (source_repo, pr_number, commit_sha) - Remove obsolete helper functions (getCommitMessage, getPRTitle, getPRBody) - Update webhook handler to pass MessageTemplater to workflow processor This fixes two critical issues: 1. Missing server files (js-express, python-fastapi) due to 100-file pagination limit 2. Blank PR descriptions with unrendered template variables
- Removed legacy configuration format (copy_rules, batch_by_repo, batch_pr_config) - Removed legacy types: CopyRule, TargetConfig, BatchPRConfig - Removed legacy fields from YAMLConfig: SourceRepo, SourceBranch, BatchByRepo, CopyRules - Removed legacy processing logic from webhook_handler_new.go - Removed legacy helper functions from pattern_matcher.go - Removed legacy test files and example configs - Updated all documentation to workflow format - Simplified config validation to only support workflows - All tests passing This is a single-user application with no backward compatibility concerns.
- Add optional 'enabled' field to WorkflowConfigRef type - Skip loading and validation of disabled workflow configs - Update main config loader to check enabled status before processing - Add test coverage for disabled workflow configs - Update example config to demonstrate enabled/disabled usage - Defaults to true when not specified for backward compatibility
- Workflows in source repo configs no longer need to specify source.repo/branch - Automatically inferred from the workflow config reference in main config - Add SourceRepo and SourceBranch context fields to WorkflowConfig - Apply context before validation via ApplySourceContext() method - Can still explicitly override source.repo or source.branch if needed - Add comprehensive test for context inference - Update example config to demonstrate simplified syntax - Reduces redundancy and makes configs cleaner
- Document source context inference in main-config-example.yaml - Add note about workflows inheriting repo/branch from references - Update env.yaml.example with comprehensive main config documentation - Add main config options to env.yaml.production as commented examples - Document flexible path support for MAIN_CONFIG_FILE - Document enabled/disabled workflow config feature - Clarify that USE_MAIN_CONFIG is auto-enabled when MAIN_CONFIG_FILE is set
- Add note that support is planned but not yet fully implemented - Clarify that infrastructure exists but needs additional work - Prevents confusion for users trying to use syntax - All example syntax remains valid and demonstrates current features
- Add custom YAML unmarshaling for transformations, commit_strategy, and exclude
- Support references to external files for reusable components
- Resolve references relative to workflow config file location
- Add comprehensive test coverage for functionality
- Update documentation to reflect is now fully implemented
Benefits:
- Share common configurations across multiple workflows
- Keep workflow configs clean and focused
- Organize related files in logical directory structure
Example usage:
transformations:
$ref: "transformations/mflix-java.yaml"
commit_strategy:
$ref: "strategies/mflix-pr-strategy.yaml"
exclude:
$ref: "common/mflix-excludes.yaml"
All tests pass (13/13 MainConfigLoader tests)
- Update config.yaml to use relative paths with ../ for - Change all commit_strategy paths from 'strategies/' to '../strategies/' - Update documentation to reflect .copier/workflows/config.yaml location - Add clear path resolution examples showing ../ usage - Update mflix-pr-strategy.yaml with correct location and usage docs This matches the user's actual directory structure: .copier/workflows/config.yaml (workflow config) .copier/strategies/mflix-pr-strategy.yaml (strategy file) Path resolution: From .copier/workflows/config.yaml Reference: ../strategies/mflix-pr-strategy.yaml Resolves to: .copier/strategies/mflix-pr-strategy.yaml
- Changed config loading to log warnings and continue instead of failing completely when a workflow config file is missing or invalid - This allows the app to process workflows from available repos even if some referenced configs are not yet created - Added test to verify resilient behavior with missing configs - Fixes issue where app would fail to process any PRs if one workflow config reference was broken
…features - Update ARCHITECTURE.md with main config system, support, source context inference, and resilient loading - Update README.md with main config examples and workflow references - Update DEPLOYMENT.md to reference Cloud Run instead of App Engine - Update LOCAL-TESTING.md with correct config file references - Remove outdated references to CONFIG_FILE (now MAIN_CONFIG_FILE) - Focus on conciseness and accuracy
- Update config-validator README to clarify it validates workflow configs, not main configs - Update test-webhook README to remove outdated CONFIG_FILE references - Update all example commands to reflect current usage - Add note that main config validation is built into the app
- Remove legacy format fallback from main_config_loader.go - Remove JSON export functions (ExportConfigAsJSON, ExportConfigAsYAML) - Remove ConfigTemplate and GetConfigTemplates functions - Remove 'convert' command from config-validator CLI tool - Remove all migration guides and legacy format references from docs - Update test to verify main config requires workflow_configs - Update FAQ to reflect current config structure - Remove all references to JSON conversion and legacy formats All tests passing (14/14 MainConfigLoader tests)
The initConfig function was still referencing services.GetConfigTemplates() and services.ConfigTemplate which were removed in the legacy code cleanup. Simplified initConfig to use a basic inline template instead.
INSTALLATION_ID is a required environment variable but was missing from the example env files. This caused deployment failures with the error: 'missing required environment variables: INSTALLATION_ID' Added INSTALLATION_ID back to: - configs/env.yaml.example - configs/env.yaml.production
5b752e4 to
d5af4a0
Compare
|
Redeployed and tested ✅ |
Jira:
Workflow-Based Configuration System
Replaces rule-centric config with workflow format, reducing config complexity by 70% while maintaining all features.
Key Changes
Bug Fixes
Config Example